-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Nested Queries #21557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Nested Queries #21557
Conversation
This should be possible, but the access checks are tricky.
…rom nested queries.
I'm not certain it's the best way of going about it but couldn't we implement this by storing the entity of the parent instead of the data and then resolving the data from the entity when it is needed? |
I'm not sure what you mean. Like, instead of yielding a let mut items = query.iter_mut().collect::<Vec<_>>();
let mapped = items.iter_mut().map(|item| item.get_mut()).collect::<Vec<_>>(); |
|
I think these changes are a great idea; I guess I would like to know how this contrasts to how flecs queries for relation data. Maybe @james-j-obrien knows? |
That's a quite involved question to answer (although a very interesting one). The big difference between the bevy and flecs queries are tied to one being defined in the type system and the other being defined dynamically. Due to this bevy queries are fundamentally based on nesting, you have tuples of query terms that each store their own state and generate their own code for managing that state. In flecs all the query terms are just stored in a flat array. For example in this PR we express querying our parent as creating a query term that traverses the relationship and then nested in that is the set of components we want to access on the target, whereas in flecs you would have a set of instructions that said: "get me any entity A with relationship of the form (ChildOf, B) and store B as a variable", "get me component Y on entity B", "get me component Z on entity B". This structure allows flecs to optimize/batch/reorder terms since they can be evaluated in the full context of the rest of the query, but for simple queries it's mostly a different path to the same goal. Since flecs also has fragmenting relations they can do stuff like cache the tables for B since you know that entities in A's table will always have parent B. All that being said, with bevy's queries as they exist today this PR seems like the shortest path to querying on multiple entities so seems like a positive step. |
…` and fix missing documentation.
…:init_nested_access`.
Thanks for the answer! I guess we could do something similar: for tuple queries, build such a node graph and use it to match entities. I guess we do already create a data structure that helps us find matching entities; that data structure is the QueryState.
And the main difference is that the flecs "QueryState" is more elaborate since it can contain sources, relationships, etc. |
|
@eugineerd, I liked your work over in #21601; can I get your review here in turn? |
|
@james-j-obrien @cBournhonesque Another difference with Flecs relationship queries, and a limitation (I think) of the solution proposed in this PR is that you cannot have constraints that span multiple terms. For example: // find all entities that like the same food that they're eating
(Likes, $food), (Eats, $food)or // Match all spaceships that are docked to a planet of the same faction that they belong to
SpaceShip, (Faction, $f), (DockedTo, $obj), Faction($obj, $f) |
|
Yes, I think we would need a more complex I think the idea was floated to have a dynamic QueryBuilder which would be transmuted to Maybe it's independent from this PR?
I guess some questions I have are:
|
Would it be possible if |
Yeah, I want to leave that for a follow-up, but I do think we should do it! My plan is to give I don't think it will be hard to do, but I think it would make the diff harder to review if it were combined with this PR. |
…access` must be called and must panic if access conflicts.
…donly` before `init_access`.
…e-access # Conflicts: # crates/bevy_ecs/src/query/world_query.rs # crates/bevy_ecs/src/world/entity_ref.rs
…e-access # Conflicts: # crates/bevy_ecs/src/query/fetch.rs # crates/bevy_ecs/src/query/iter.rs # crates/bevy_ecs/src/query/mod.rs # crates/bevy_render/src/sync_world.rs
| unsafe impl<D: QueryData + 'static, F: QueryFilter + 'static> WorldQuery for NestedQuery<D, F> { | ||
| type Fetch<'w> = NestedQueryFetch<'w>; | ||
| // Ideally this would be `QueryState<D, F>`, | ||
| // but `QueryData` requires `Self::State == Self::ReadOnly::State`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove this bound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove this bound?
Hmm, I think we could, but I don't think it's worth it for this PR.
The goal is to be able to cheaply convert &QueryState<D, F> to &QueryState<D::ReadOnly, F> during Query::as_readonly() for things like Query::iter(). That's done by making QueryState be #[repr(transparent)] and only containing D::State and F::State, which means QueryState<D, F> and QueryState<D::ReadOnly, F> are guaranteed to be transmutable.
We could technically drop the constraint and have a safety requirement that D::State be transmutable to F::State. But that would make it easier to get wrong, and the transmutes are already tricky to justify.
Another approach would be to rework it like
#[repr(transparent)]
struct QueryState<D, F> {
inner: QueryStateInner<D::State, F::State>,
}So that we have a type parameterized by the state types instead of the query types themselves. Then we could do type State = QueryStateInner<D::State, F::State> and it would work. That would also let us justify the transmutes without the need for #[repr(c)], letting the compiler optimize the layout slightly. But that would be a pretty extensive refactor!
I'd be inclined to see what happens to QueryState with #21607 and #19787 before trying to clean this up more. If QueryState winds up behind a trait bound, we might have more room to maneuver here.
| unsafe { QueryState::<D, F>::new_unchecked(world).into_readonly() } | ||
| } | ||
|
|
||
| fn get_state(_components: &Components) -> Option<Self::State> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is get_state only used for World::try_query where we try to create a query using an immutable reference to world?
Could we change the API to take &World as input, and then call QueryState::try_new()?
Although apparently allowing &World allows UB: #13343
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
get_stateonly used forWorld::try_querywhere we try to create a query using an immutable reference to world? Could we change the API to take&Worldas input, and then callQueryState::try_new()?Although apparently allowing
&Worldallows UB: #13343
It's also used by query transmutes and joins, like transmute_lens. That's where the UB comes in: transmute_lens is called with the access of the source query, which wouldn't have access to the DefaultQueryFilters resource that's needed to create a new QueryState.
I think the ideal behavior would be to build the nested query state by transmuting the nested query state from the source query. Like, transmuting Query<Parent<(&T, &U)>> to Query<Parent<&T>> would still only match entities whose parent has both T and U. Then we wouldn't need to re-evaluate DefaultQueryFilters. But I don't see any good way to match up the nested queries in the source with those in the target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to pursue multi-source queries using a different approach (dynamic query plans similar to what flecs is doing), but I think that the traits introduced are broadly useful.
It will also be nice to have a shorter path towards relation queries and to gain more experience in usage/API design related to them.
Awesome work!
FWIW, my opinion is that both approaches will be useful and can happily co-exist :) I do think that this is a key missing feature for bevy_ecs, regardless of what route we take to get there. |
…e-access # Conflicts: # crates/bevy_ecs/src/query/state.rs
Objective
Support queries that soundly access multiple entities.
This can be used to create queries that follow relations, as in #17647.
This can also be used to create queries that perform resource access. This has been supported since #16843, although that approach may become unsound if we do resources-as-components #19731, such as #21346.
Fixes #20315
Solution
Allow a
QueryDatathat wants to access other entities to store aQueryState<D, F>in itsWorldQuery::State, so that it can create a nestedQuery<D, F>during the outerfetch.NestedQuerytypeIntroduce a
NestedQuerytype that implementsQueryDataby yielding aQuery. It is intended to be used inside other implementations ofQueryData, either for manual implementations or#[derive(QueryData)]. It is not normally useful to query directly, since it's equivalent to adding anotherQueryparameter to a system.In theory, we could directly
impl QueryData for Query, but this would be too easy to do accidentally. Having to explicitly import and writeNestedQuerywill make it clear that it's something unusual, and also allows us to remove the need for passing'staticfor the'wand'slifetimes.New
WorldQuerymethodsFor it to be sound to create the
Queryduringfetch, we need to register theFilteredAccessof the nested query and check for conflicts with other parameters. Create aWorldQuery::update_external_component_accessmethod for that purpose. ForQuery as SystemParam, call this duringinit_accessso the access can be combined with the rest of the system access. For looseQueryStates, call it duringQueryState::new.In order to keep the query cache up-to-date, create a
WorldQuery::update_archetypesmethod where it can callQueryState::update_archetypes_unsafe_world_cell, and call it from there.New
QueryDatasubtraitsSome operations would not be sound with nested queries! In particular, we want a
Parent<D>query that reads data from the parent entity by following theChildOfrelation. But many entities may share a parent, so it's not sound to iterate aQuery<Parent<&mut C>>.It is sound to
get_mut, though, so we want the query type to exist, just not be iterable. And following the relation in the other direction for aQuery<Children<&mut C>>is sound to iterate, since children are unique to a given parent.So, introduce two new
QueryDatasubtraits:IterQueryData- For anything it's sound to iterate. This is used to bounditer_mutand related methods.SingleEntityQueryData- For anything that only accesses data from one entity. This is used to boundEntityRef::get_components(see Unsound to callEntityRef::get_componentswith aQueryDatathat performs resource access #20315). It's also used to boundtransmuteat the moment, although we may be able to relax that later (see below, under Future Work).Note that
SingleEntityQueryData: IterQueryData, since single-entity queries never alias data across entities, andReadOnlyQueryData: IterQueryData, since it's always sound to alias read-only data.Here is a summary of the traits implemented by some representative
QueryData:&T&mut TParent<&T>Parent<&mut T>(&mut T, Parent<&U>)Children<&mut T>Alternatives
We could avoid the need for the
IterQueryDatatrait by making it a requirement for allQueryData. That would reduce the number of traits required, at the cost of making it impossible to supportQuery<Parent<&mut C>>.Showcase
Here is an implementation of a
Related<R, D, F>query using this PR:I'd like to leave that to a follow-up PR to allow bikeshedding the API, and to take advantage of #21581 to remove the
Option, but I think it works!Future Work
There is more to do here, but this PR is already pretty big. Future work includes:
WorldQuerytypes for working with relationships #17647AssetChangedto use nested queries for resource access, and stop tracking resource access separately inAccessget_stateforNestedQuery. This is difficult because constructing aQueryStaterequires reading theDefaultQueryFiltersresource, butget_statecan be called fromtransmutewith no access.SingleEntityQueryDatabound on transmutes and joins. This will require checking that the nested query access is also a subset of the original access. Although unless we also solve the problem of implementingget_state, transmuting to a query with nested queries won't work anyway.QueryIterby offering afn fetch_next(&self) -> D::Item<'_>method and relaxing theIterQueryDatabound onQuery::into_iterandQuery::iter_mut. This would work similar toiter_many_mutanditer_many_inner.IterQueryDatabound onQuery::single_inner,Query::single_mut, andSingle<D, F>. This seems like it should be straightforward, because the method only returns a single item. But the way it checks that there is only one item is by fetching the second one!